Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added-binary-arithmetic-operators-logical-operators-topk #723

Closed
wants to merge 1 commit into from

Conversation

HarshitNagpal29
Copy link

Modified the 6_loadgen.yaml file to include tests for the additional PromQL features mentioned in the issue:

  1. Binary Arithmetic Operators (Joins)
  2. Logical Operators (and, or, unless)
  3. TopK

@HarshitNagpal29
Copy link
Author

HarshitNagpal29 commented Aug 22, 2024

@bboreham sir, can you kindly approve it so that it can be merged?

@SuperQ SuperQ requested a review from bboreham August 30, 2024 05:58
@SuperQ
Copy link
Member

SuperQ commented Aug 30, 2024

This needs a DCO sign-off. You can use git commit -s --amend to add it.

Copy link
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for this, it's structurally in the right direction but probably not great choices of metrics.

interval: 15s
type: instant
queries:
- expr: sum(node_memory_MemAvailable_bytes) / sum(node_memory_MemTotal_bytes)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked a running Prombench, and there are only 3 series like this; perhaps not great for a benchmark.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bboreham Hi Sir, I hope you are doing well. First of all, sorry for the late reply. I'm also really glad that you liked my #743 PR.

So, now that I'm focusing on this, here are some of the suggested changes from my side. PTAL once so I can create a commit and merge it.

- name: binary_arithmetic_operators
  interval: 15s
  type: instant
  queries:
  - expr: sum(rate(codelab_api_requests_total[5m])) / count(codelab_api_requests_total)
  - expr: rate(codelab_api_request_duration_seconds_count[5m]) * 60
  - expr: codelab_api_request_duration_seconds_sum / codelab_api_request_duration_seconds_count

As suggested by you, I have made changes, PTAL.

interval: 15s
type: instant
queries:
- expr: topk(5, rate(http_requests_total[5m]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find any of these in Prombench. Where would this metric come from?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more proposed changes:

- name: logical_operators
  interval: 15s
  type: instant
  queries:
  - expr: rate(codelab_api_requests_total{method="GET"}[5m]) and rate(codelab_api_requests_total{method="POST"}[5m])
  - expr: codelab_api_requests_total unless codelab_api_request_errors_total
  - expr: rate(codelab_api_requests_total[5m]) or rate(codelab_api_request_errors_total[5m])
 
- name: topk_example
  interval: 15s
  type: instant
  queries:
  - expr: topk(5, sum by(method, path) (rate(codelab_api_requests_total[5m])))

@bboreham
Copy link
Member

For reference, I copied this from http://prombench.prometheus.io/14920/prometheus-release/tsdb-status:

Top 10 series count by metric names

Name Count
codelab_api_request_duration_seconds_bucket 5005000
codelab_api_request_duration_seconds_sum 192500
codelab_api_requests_total 192500
codelab_api_request_duration_seconds_count 192500
codelab_api_request_errors_total 96250
go_gc_duration_seconds 68780
scrape_samples_post_metric_relabeling 13759
scrape_samples_scraped 13759
scrape_duration_seconds 13759
scrape_series_added 13759

@HarshitNagpal29
Copy link
Author

@bboreham Hi Sir, hope you are doing well, thanks for your reply, currently I am having my university exams till the end of this month, so not able to look into this matter for now, but as soon as it will end, I will review it properly and make appropriate changes. Hope you understand.

@bboreham
Copy link
Member

No problem; I think @kushalShukla-web is going to take a look.

@bboreham
Copy link
Member

This was overtaken by #747. Thanks for your contribution!

@bboreham bboreham closed this Oct 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants